Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Common - Optimize CBA_fnc_compatibleMagazines #1585

Merged
merged 5 commits into from
Sep 7, 2023

Conversation

johnb432
Copy link
Contributor

When merged this pull request will:

@commy2
Copy link
Contributor

commy2 commented Jul 11, 2023

If it's cached anyway, why bother dicking around with dictionaries when building up the arrays the first time? This getOrDefaultCall syntax is an abomination.

I like the namespace -> HashMap part though.

Any benchmark before after?

@johnb432
Copy link
Contributor Author

johnb432 commented Jul 12, 2023

If it's cached anyway, why bother dicking around with dictionaries when building up the arrays the first time?

Because it's faster. You don't have to use pushBackUnique, which has to verify the entire array before pushing or not. Same goes for arrayIntersect on an array on itself. With the hashmap if something is already in it, you overwrite it and it doesn't matter.

This getOrDefaultCall syntax is an abomination.

The reason I used that is because it's faster.

Any benchmark before after?

I will run some tonight and report the numbers, but from the limited testing I ran it's always faster.

@johnb432
Copy link
Contributor Author

I like the namespace -> HashMap part though.

Something I have just thought of: I could make the hashmap stored in uiNamespace, instead of refreshing the cache on mission start.
Is this something that would be wanted?

@commy2
Copy link
Contributor

commy2 commented Jul 12, 2023

No, that can be abused.

@johnb432
Copy link
Contributor Author

I tested with "arifle_AK12_F" in a big modset.

In a first test, I ran only the code from CBA_fnc_compatibleMagazines that gets the magazines (from Line 38 to Line 61 and from Line 37 to Line 65). I wanted to see how fast the functions would get the magazines if the weapon wasn't cached. This is what I got:

["arifle_AK12_F"] call _oldCode; // 0.70 ms
["arifle_AK12_F"] call _newCode; // 0.20 ms

I reran the first test, but with this time to get all magazines. However, these do call CBA_fnc_compatibleMagazines, which was the new code (which means that the old benefitted from the new hashmap caching during this specific test).

["arifle_AK12_F", true] call _oldCode; // 0.25 ms
["arifle_AK12_F", true] call _newCode; // 0.0475 ms

So for getting magazines the first time, the new code is significantly faster.

For conventional usage:

"arifle_AK12_F" call CBA_fnc_compatibleMagazines // Old, 0.0070 ms
"arifle_AK12_F" call CBA_fnc_compatibleMagazines // New, 0.0067 ms

["arifle_AK12_F", true] call CBA_fnc_compatibleMagazines // Old, 0.0073 ms
["arifle_AK12_F", true] call CBA_fnc_compatibleMagazines // New, 0.0070 ms

// Much smaller modset:
"arifle_AK12_F" call CBA_fnc_compatibleMagazines // Old, 0.0051 ms
"arifle_AK12_F" call CBA_fnc_compatibleMagazines // New, 0.0046 ms

["arifle_AK12_F", true] call CBA_fnc_compatibleMagazines // Old, 0.0053 ms
["arifle_AK12_F", true] call CBA_fnc_compatibleMagazines // New, 0.0048 ms

No, that can be abused.

How so? How is having a uiNamespace variable more prone to abuse than a missionNamespace variable?

@commy2
Copy link
Contributor

commy2 commented Jul 12, 2023

How so? How is having a uiNamespace variable more prone to abuse than a missionNamespace variable?

You can put compileFinal'd function in there before it is populated by an actual value and therefore can cause scripts to fail unexpectedly.

missionNamespace is cleared at mission start, therefore can't carry over values from, e.g. the editor.

@commy2
Copy link
Contributor

commy2 commented Jul 12, 2023

So for getting magazines the first time, the new code is significantly faster.

That's irrelevant then though. .X milliseconds once per mission is a meaningless improvement. Readability trumps premature optimization here.

The relevant result is pretty much the same. ~5-10% improvement. Not worth it.

@johnb432
Copy link
Contributor Author

You can put compileFinal'd function in there before it is populated by an actual value and therefore can cause scripts to fail unexpectedly.

missionNamespace is cleared at mission start, therefore can't carry over values from, e.g. the editor.

If you have a mod loaded that loads anything in cba_jr_namespace (missionNamespace or uiNamespace) besides CBA, then imo it's for the better that the game will throw errors, as you will be forced to unload that mod in order to play. The only reason I could see why cba_jr_namespace would have anything assigned to it from another mod is malicious intent. The variable name is too specific to be accidentally used by another mod.

If there is a will, there is a way. If people want to break something, they will be able to. You could just as easily break cba_jr_namespace in missionNamespace as in uiNamespace.


That's irrelevant then though. .X milliseconds once per mission is a meaningless improvement.

The relevant result is pretty much the same. ~5-10% improvement. Not worth it.

How is it irrevelant, meaningless or not worth it? Arma runs like a piece of shit. Any improvement should be a welcome one.

Readability trumps premature optimization here.

I seriously fail to see how the readability of the code has been compromised. Just when comparing the difference side by side you can see it's not fundamentally different.
I'm sorry to hear you don't like getOrDefaultCall, but it's meant for this exact purpose. The code could not be cleaner imo: The engine does a lot internally.

"Premature" really rubs me the wrong way. What is the premature part?
The only premature part I see is that the first entry of CBA_fnc_compatibleMagazines' return is not the first magazine listed in the magazines array of a weapon, which is considered to be the default by e.g. the arsenal. So from that point of view I could understand the premature comment.
However, you didn't point that out, so I imagine you are referring to something else.

@commy2
Copy link
Contributor

commy2 commented Jul 14, 2023

then imo it's for the better that the game will throw errors

Errors can be ignored. The failed script however may give the player an ingame advantage. Yes, malicious intend.

I have reported bugs of ui namespace abuse to BI multiple times in the past, and they have all fixed them. Even got me two extra copies of APEX as reward, because some of these enabled arbitrary code execution.

It's hard to utilize ui namespace without introducing exploits. It's not worth it to work around these exploits when the potential gain is a laughable < 0.001 seconds per mission start per weapon. That's what makes any of this premature.

"Premature" really rubs me the wrong way. What is the premature part?
The only premature part I see is that the first entry of CBA_fnc_compatibleMagazines' return is not the first magazine listed in the magazines array of a weapon, which is considered to be the default by e.g. the arsenal. So from that point of view I could understand the premature comment.

"Premature optimization" is a phrase coined by Donald Knuth.

The cache itself is properly utilized optimization. It made using the function essentially free. I still like the part where you rewrite it to use the native HashMap instead of the custom CBA namespace. I have nothing against modernizing old code.

@johnb432
Copy link
Contributor Author

I still like the part where you rewrite it to use the native HashMap instead of the custom CBA namespace. I have nothing against modernizing old code.

Ultimately, what does this mean? What changes are going to be accepted besides switching GVAR(magNamespace) to be a hashmap? What about getOrDefaultCall - yay or nay?

@commy2
Copy link
Contributor

commy2 commented Jul 15, 2023

What changes are going to be accepted besides switching GVAR(magNamespace) to be a hashmap?

If it were up to me, that + the function header fixes.

What about getOrDefaultCall - yay or nay?

nay, not worth it

@johnb432
Copy link
Contributor Author

If it were up to me, that + the function header fixes.

If nobody else says anything in the near future, I'll go with what you said.

@johnb432
Copy link
Contributor Author

If it were up to me, that + the function header fixes.

Seeing as nobody has said anything, going ahead with your suggestion.

Copy link
Contributor

@commy2 commy2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@jonpas jonpas added this to the 3.16.0 milestone Sep 6, 2023
@jonpas jonpas changed the title Common - Updated CBA_fnc_compatibleMagazines Common - Optimize CBA_fnc_compatibleMagazines Sep 6, 2023
@jonpas jonpas merged commit 9c9db2a into CBATeam:master Sep 7, 2023
4 checks passed
@johnb432 johnb432 deleted the update_CBA_fnc_compatibleMagazines branch August 14, 2024 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants